Allow NgModelController to be copied #15836
Conversation
src/ng/directive/ngModel.js
Outdated
| this.$$scope = $scope; | ||
| //https://github.com/angular/angular.js/issues/15833 | ||
| //Prevent `$$scope` from being iterated over by `copy` when NgModelController is deep watched | ||
| //TODO: remove this or do not merge into 1.7 |
There was a problem hiding this comment.
We shouldn't merge into 1.7, so you can remove this.
There was a problem hiding this comment.
I've dropped this line.
test/ng/compileSpec.js
Outdated
| expect(componentScope.undi).toBeDefined(); | ||
| //TODO: the `.not` should be removed in 1.7 due to | ||
| //https://github.com/angular/angular.js/commit/fd4f0111188b62773b99ab6eab38b4d2b5d8d727 | ||
| expect(componentScope.undi).not.toBe($rootScope.f.i); |
There was a problem hiding this comment.
I think this line is redundant (it doesn't add anything to the test). Could we use toEqual() (which would work the same in 1.6.x/1.7.x)?
There was a problem hiding this comment.
If we do keep this line I'd prefer .toBe in 1.7 to verify that it is the exact same instance, so I figured .not.toBe would be better for now, if we keep this line.
Only the $compile line is actually necessary to reproduce the issue. Maybe I should drop everything else?
There was a problem hiding this comment.
MY reasoning is that there are other tests that verify the toBe() behavior (for 1.7), so I wouldn't be too worried about it here. But dropping everything not necessary for the test works for me too.
There was a problem hiding this comment.
Ok I've dropped everything except the verification that the binding actually got set...
jbedard
force-pushed
the
15833
branch
2 times, most recently
from
f0b3776 to
f5eb920
Compare
|
LGTM, please squash and merge |
test/ng/compileSpec.js
Outdated
| @@ -5858,6 +5858,29 @@ describe('$compile', function() { | |||
| expect(componentScope.owRef).toEqual({name: 'lucas', item: {name: 'martin'}}); | |||
| })); | |||
|
|
|||
| //https://github.com/angular/angular.js/issues/15833 | |||
There was a problem hiding this comment.
Could we start every comment with a space?
src/ng/directive/ngModel.js
Outdated
| @@ -281,7 +281,9 @@ function NgModelController($scope, $exceptionHandler, $attr, $element, $parse, $ | |||
|
|
|||
| this.$$currentValidationRunId = 0; | |||
|
|
|||
| this.$$scope = $scope; | |||
| //https://github.com/angular/angular.js/issues/15833 | |||
| //Prevent `$$scope` from being iterated over by `copy` when NgModelController is deep watched | |||
There was a problem hiding this comment.
Can we have spaces after // in above two lines?
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix
What is the current behavior? (You can also link to an open issue here)
copythrows when copying aNgModelControllerinstance. This will occur when deep watching such as when doing<e ref="[myNgModel]">(#15833)What is the new behavior (if this is a feature change)?
The internal
NgModelController$$scopeis hiddenDoes this PR introduce a breaking change?
no, unless someone is doing something crazy that depends on
$$scopebeing enumerable?Please check if the PR fulfills these requirements
Other information:
I've split the commit up into 3. I think the refactor + (modified) test should go into 1.7. I don't think the actual fix should go into 1.7. The specific test case (watched a literal containing an ng-model) will no longer fail in 1.7. If we want to prevent other cases like manually deep-watching or manually
copying an ng-model I think we should do it differently in 1.7...